OnBehalfOf authenticator and token generator#3179
OnBehalfOf authenticator and token generator#3179peternied merged 2 commits intoopensearch-project:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3179 +/- ##
==========================================
Coverage 62.51% 62.51%
- Complexity 3353 3402 +49
==========================================
Files 254 259 +5
Lines 19732 20055 +323
Branches 3334 3370 +36
==========================================
+ Hits 12336 12538 +202
- Misses 5767 5866 +99
- Partials 1629 1651 +22
|
cwperks
left a comment
There was a problem hiding this comment.
Thank you @RyanL1997. I took a first pass at the review and left a few comments.
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
9b57e1b to
6cafc19
Compare
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/ssl/util/ExceptionUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtilTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
Outdated
Show resolved
Hide resolved
c43a2e9 to
9ce36dc
Compare
DarshitChanpura
left a comment
There was a problem hiding this comment.
Looks really close. Just a few comments. Also there are some codecov comments that might need addressing
src/integrationTest/java/org/opensearch/test/framework/OnBehalfOfConfig.java
Outdated
Show resolved
Hide resolved
@DarshitChanpura, Yes, I have created an exception called |
cwperks
left a comment
There was a problem hiding this comment.
Took another pass and left a few more comments. Thank you for adding tests that show the obo endpoint authorization!
src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/support/ConfigConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/user/AuthCredentials.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java
Outdated
Show resolved
Hide resolved
|
Thanks for putting this out @RyanL1997 - there might be debates such as around naming - I don't think we need perfect names, but lets try to be really solid on the customer facing interfaces and APIs. As long as we've got buy in from a couple folks lets try to iterate quickly. |
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
cwperks
left a comment
There was a problem hiding this comment.
I left a few more comments, but this looks ready to me. All comments left were nits. Thank you for filing a follow up issue to abstract the logic to determine if the obo configuration is valid.
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Show resolved
Hide resolved
|
@RyanL1997 Looks like you've got a DCO failure, can you resolve that? |
|
@RyanL1997 Could you check over all the unresolved comments and make sure they are addressed and resolved? Once there are 0 unresolved comments (resolutions like won't fix, defer to an issue, or make a change), I'll be happy to approve. |
DarshitChanpura
left a comment
There was a problem hiding this comment.
Looks ready to me. Generic comment: There are multiple usages of Exception and RuntimeException being thrown. I would suggest using OpenSearchException in place of these.
None of these comments are blockers and can be followed up in a separate PR. Once all comments are resolved I'll approve the PR. Thank you @RyanL1997 for putting this feature together and for your patience working through the comments.
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/user/AuthCredentials.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
Outdated
Show resolved
Hide resolved
Hate to say, but I can't :(. First, according to the above log from DCO, instead of expecting my correct email, it expected my email to be |
|
@RyanL1997 IMO the fastest way to fix the DCO is like this, it will require a force-push to your branch, but its pretty straight forward |
ed4c0ff to
fa0199d
Compare
Signed-off-by: Ryan Liang <jiallian@amazon.com>
fa0199d to
5e68d80
Compare
|
Thank you for the quick follow-up @RyanL1997 ! There's nothing left to address from me. Any follow-ups have already been captured in issues. |
src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Liang <jiallian@amazon.com>
DarshitChanpura
left a comment
There was a problem hiding this comment.
LGTM. Thank you @RyanL1997 . All my outstanding comments have been addressed.
Description
Merge OBO Authentication into 'main' branch
New Feature
Major Components:
JwtVendor: assemble the jwt based OBO token.OnBehalfOfAuthenticator: the authentication backend for validate OBO TokenCreateOnBehalfOfTokenAction: the endpoint of creation of OBO TokenMajor Testing Components:
JwtVendorTest: Unit tests for JwtVendorOnBehalfOfAuthenticatorTest: Unit tests for OBO authentication backendOnBehalfOfAuthenticationTest: Integration test for OBO issuing endpointsIssues Resolved
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.